RE: Enable Whitelist Stargate Query #2353
Conversation
This reverts commit eb216a1.
There was a problem hiding this comment.
Overall looks good, nice work!
Could you please elaborate in comments/specs on the following things:
- why do we need to proto unmarshal and then json marshal?
- why do we need to
message.Reset()?
I also think that we might consider splitting up some test cases and testing StargateQuerier directly
|
Why do we redefine Shouldn't we encourage clients to define their own? |
Having clients define their own was the previews solution. Upstreaming the whitelist means that very few clients will need to define their won (they can just whitelist endpoints instead) |
Yeah also having clients define their own was too much overhead, almost impossible for clients to use. |
|
Sounds good, makes sense @mattverse @nicolaslara . Then, why are we defining our own instead of merging the generic Should we then completely move all the |
|
On testing, do we have list of known causes for non-determinism? Notice that there seems to be only schema upgrade test that address non-determinism issue. If we have the list, then we might be able to create a generic test scenario to test against all enabled stargate query and construct test from Other point is what I pointed out to @mattverse few weeks back about the potential of making whitelist a config file of some sort. Is that possible? Will need that to filter out CosmWasm Querier generation, but if it's too tricky, I could make code gen retrieve all keys from |
I don't think there is a comprehensive list, and there could always be new, previously unknown causes of non-determinism that pop-up. The main culprits at the moment are returning values based on: explicit randomness, any sort of unordered lists, system time, floats, and maps.
I think doing these generic tests for the causes of non-determinism would be quite difficult (they're all quite unique), but great if achievable! maybe some sort of simulation with different underlying architectures/setups for the nodes? Could be done with e2e testing, but I'd say it's outside the scope of this PR. |
|
Added way more test cases upon @p0mvn 's review! |
Synched with Matt. The goal is to eventually remove |
There was a problem hiding this comment.
LGTM! Great work @mattverse . Thanks for testing this thoroughly
Co-authored-by: Roman <roman@osmosis.team>
There was a problem hiding this comment.
LGTM! Great work @mattverse!
Wish the whitelist process doesn’t require registering response type but that could be future improvement.
* Add stargate queries * Fix test * Remove initial whitelist queries * Add initial bindings, change names * Remove cosmos protos * Fix lint * Update wasm binary * Revert "Update wasm binary" This reverts commit eb216a1. * Bez's review * Add test cases * Remove logs * WIP: return json marshalled response * Add Test Case * Add changelog * Modify Changelog * Normalize -> ProtoToJson * Fix merge conflict * Add stargate querier test * Add alot more test cases * Add test case for breaking grpc querier * Update wasmbinding/query_plugin_test.go Co-authored-by: Roman <roman@osmosis.team> * code review from upstream Co-authored-by: Roman <roman@osmosis.team> (cherry picked from commit d5cc792)
Closes: #2190
Closes: #2098
What is the purpose of the change
Add whitelisted stargate queries and ensure deterministic query. The difference of this PR with the previous PR(#2190) is that we json marshal the responses, not proto marshalling them. Test cases have been also been updated for this accordingly, tested and confirmed that this way of enabling stargate queries work via sample contract (https://github.com/osmosis-labs/swaprouter)
Brief Changelog
Testing and Verifying
Tested locally and confrimed that this way of whitelisting works contract side by deploying contract with this commit.
Also added go test to ensure we're safe with json marshalling.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes